Add CLI helpers for managing schedules#97
Conversation
bradhe
requested review from
Copilot,
jo-sm,
konstantinoscs,
sammuti and
socksy
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive CLI functionality for managing schedules in Tower applications. The implementation provides CRUD operations for schedules with support for cron expressions and runtime parameters.
Key changes include:
- New CLI module with commands for creating, listing, updating, and deleting schedules
- API integration functions for all schedule operations with proper error handling
- Command-line argument parsing for schedule parameters and filters
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/tower-cmd/src/schedules.rs | Implements CLI commands and handlers for schedule management operations |
| crates/tower-cmd/src/lib.rs | Integrates the schedules module into the main CLI application |
| crates/tower-cmd/src/api.rs | Adds API client functions for schedule operations with response handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
crates/tower-cmd/src/schedules.rs
Outdated
| if cron.is_none() { | ||
| output::die("You must specify a cron string (--cron) for this schedule"); | ||
| } |
There was a problem hiding this comment.
The update command requires a cron parameter but doesn't allow updating other fields independently. Consider making cron optional in updates to allow partial updates of schedule properties.
| if cron.is_none() { | |
| output::die("You must specify a cron string (--cron) for this schedule"); | |
| } | |
| // Allow partial updates: cron is optional. |
There was a problem hiding this comment.
I suppose you might wanna update the parameters instead of the cron.
crates/tower-cmd/src/schedules.rs
Outdated
| match serde_json::from_str::<HashMap<String, String>>(params_str) { | ||
| Ok(params) => Some(params), | ||
| Err(_) => { | ||
| output::die("Invalid parameters JSON format. Expected object with string key-value pairs."); |
There was a problem hiding this comment.
I wonder if we may want to change that in the future... It is common in CLJ world to have env variable config be able to autodetect numbers and keywords etc and coerce them, surely something similar must exist in the Python world? In which case I'd be annoyed I had to specify them as strings in the JSON (even though behind the scenes we'd just coerce them into strings before passing them in as env variables anyhow).
| } | ||
| } | ||
|
|
||
| pub async fn do_update(config: Config, args: &ArgMatches) { |
There was a problem hiding this comment.
👍 I really like this naming pattern of having the outer, side effectful user interaction be do_something that then calls the pure some_the_thing inside. This makes it easier to adapt for the MCP server, where I discovered that e.g. getting exposed to a spinner is a very unfortunate thing to go to an LLM.
crates/tower-cmd/src/schedules.rs
Outdated
| .short('e') | ||
| .long("environment") | ||
| .value_parser(value_parser!(String)) | ||
| .required(true) |
There was a problem hiding this comment.
I'm not sure it should be required as an argument here. I guess we're already doing this elsewhere so it makes sense to continue in this PR, but I would expect to have it go to the default environment, or to be able to set it in other ways than the CLI arg (env var, config file)
There was a problem hiding this comment.
Ah, yes, good point--we use "default" everywhere else. I'll update this.
konstantinoscs
deleted the
feature/tow-512-add-new-schedule-editor-to-tower-cli
branch
This PR introduces CLI helpers for managing schedules.